[GOBBLIN-2158] upgraded gson version with unit test #4057

Blazer-007 · 2024-09-17T15:47:55Z

Dear Gobblin maintainers,

Please accept this PR. I understand that it will not be reviewed until I have checked off all the steps below!

JIRA

[✅] My PR addresses the following Gobblin JIRA issues and references them in the PR title. For example, "[GOBBLIN-XXX] My Gobblin PR"
- https://issues.apache.org/jira/browse/GOBBLIN-2158

Description

[✅] Here are some details about my PR, including screenshots (if applicable):
- Upgraded Gson version to 2.8.9

Tests

[✅] My PR adds the following unit tests OR does not need testing for this extremely good reason:
- testObjectToIntegerDeserialize

Commits

[✅] My commits all reference JIRA issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "How to write a good git commit message":
1. Subject is separated from body by a blank line
2. Subject is limited to 50 characters
3. Subject does not end with a period
4. Subject uses the imperative mood ("add", not "adding")
5. Body wraps at 72 characters
6. Body explains "what" and "why", not "how"

gobblin-api/src/test/java/org/apache/gobblin/util/io/GsonInterfaceAdapterTest.java

gobblin-api/src/main/java/org/apache/gobblin/util/io/GsonInterfaceAdapter.java

pratapaditya04 · 2024-09-18T05:15:38Z

gobblin-api/src/main/java/org/apache/gobblin/util/io/GsonInterfaceAdapter.java

+        String value = in.nextString();
+        try {
+          return Integer.parseInt(value);
+        } catch (NumberFormatException var3) {


nit : a better way for naming exceptions in this scenario could be ignored 2/3, instead of var 2/3.

pratapaditya04 · 2024-09-18T05:17:06Z

gobblin-api/src/main/java/org/apache/gobblin/util/io/GsonInterfaceAdapter.java

+            try {
+              Double d = Double.valueOf(value);
+              if ((d.isInfinite() || d.isNaN()) && !in.isLenient()) {
+                throw new MalformedJsonException("JSON forbids NaN and infinities: " + d + "; at path " + in.getPath());


nit: prefer using String.format to improve readability and performance, as String concatenation using + can create multiple intermediate String objects, which impacts performance/
For ex sample replacement :
throw new JsonParseException(String.format("Cannot parse %s; at path %s", value, in.getPath()), var1);

pratapaditya04 · 2024-09-18T05:18:48Z

gobblin-api/src/test/java/org/apache/gobblin/util/io/GsonInterfaceAdapterTest.java

@@ -43,4 +44,58 @@ public void test() {

  }

+  @Test


Test for null/infinites are missing

pratapaditya04 · 2024-09-18T05:22:02Z

gobblin-api/src/main/java/org/apache/gobblin/util/io/GsonInterfaceAdapter.java

+        } catch (NumberFormatException var3) {
+          try {
+            return Long.parseLong(value);
+          } catch (NumberFormatException var2) {


Nice work on the custom number parsing. But this includes a lot of nesting which could imapact readibility and maintainability , I sugges tbreaking down the nested try-catch blocks in readNumber into smaller methods for each number type (e.g., tryParseInteger, tryParseLong, tryParseDouble). This will improve readability and maintainability by reducing nesting.
Sample code

`private Number parseNumber(String value, JsonReader in) throws IOException { Number number = tryParseInteger(value); if (number != null) return number; number = tryParseLong(value); if (number != null) return number; return tryParseDouble(value, in); } private Number tryParseInteger(String value) { /* ... */ } private Number tryParseLong(String value) { /* ... */ } private Number tryParseDouble(String value, JsonReader in) throws IOException { /* ... */ }

`

pratapaditya04 · 2024-09-18T05:25:28Z

gobblin-api/src/main/java/org/apache/gobblin/util/io/GsonInterfaceAdapter.java

    return gson;
  }
+
+  public enum CustomToNumberPolicy implements ToNumberStrategy {


nit : minor suggestion on naming :CustomToNumberPolicy sounds a bit generic, can be renamed to something like GsonNumberParsingPolicy to reflect its purpose more clearly
.

The library implementation is
public enum ToNumberPolicy implements ToNumberStrategy

Since my code is wrapper on top of this that's the reason for keeping it generic

It can be something like LongIntNumberPolicy

Blazer-007 · 2024-09-18T05:28:23Z

@pratapaditya04 this is the default implementation of LONG_OR_DOUBLE enum policy present in the library

LONG_OR_DOUBLE {
  @Override public Number readNumber(JsonReader in) throws IOException, JsonParseException {
    String value = in.nextString();
    try {
      return Long.parseLong(value);
    } catch (NumberFormatException longE) {
      try {
        Double d = Double.valueOf(value);
        if ((d.isInfinite() || d.isNaN()) && !in.isLenient()) {
          throw new MalformedJsonException("JSON forbids NaN and infinities: " + d + "; at path " + in.getPath());
        }
        return d;
      } catch (NumberFormatException doubleE) {
        throw new JsonParseException("Cannot parse " + value + "; at path " + in.getPath(), doubleE);
      }
    }
  }
},

I have tried to just add one more layer on top to parse Int that's why exception and naming i have kept similar to library implementation

pratapaditya04 · 2024-09-18T05:34:13Z

@pratapaditya04 this is the default implementation of LONG_OR_DOUBLE enum policy present in the library

LONG_OR_DOUBLE {
  @Override public Number readNumber(JsonReader in) throws IOException, JsonParseException {
    String value = in.nextString();
    try {
      return Long.parseLong(value);
    } catch (NumberFormatException longE) {
      try {
        Double d = Double.valueOf(value);
        if ((d.isInfinite() || d.isNaN()) && !in.isLenient()) {
          throw new MalformedJsonException("JSON forbids NaN and infinities: " + d + "; at path " + in.getPath());
        }
        return d;
      } catch (NumberFormatException doubleE) {
        throw new JsonParseException("Cannot parse " + value + "; at path " + in.getPath(), doubleE);
      }
    }
  }
},

I have tried to just add one more layer on top to parse Int that's why exception and naming i have kept similar to library implementation

Hmmm, but still 3 layers of nesting impacts readability a lot, so breaking down into smaller functions can be a possible alternative,but leaving it upto you on how to handle it. Also naming exception variables var3, var2, doesn't seem right.

Will-Lo · 2024-09-18T19:49:31Z

Can we update the PR description to add the motivation of the change around custom number parsing and the gson version bump?

Blazer-007 · 2024-09-19T10:19:46Z

Closing this PR as writing custom NumberPolicy is leading to incorrect type conversion between INT & LONG in case value of object lies in range of int then there is no way to differentiate whether this object (currently in form of json string) is INT or LONG, so the required result is not achieved.

upgraded gson version with unit test

e2d921a

khandelwal-prateek reviewed Sep 17, 2024

View reviewed changes

addressed review comments

8ccdba4

pratapaditya04 reviewed Sep 18, 2024

View reviewed changes

Blazer-007 closed this Sep 19, 2024

Blazer-007 deleted the virai_upgrad_gson_version branch September 19, 2024 10:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[GOBBLIN-2158] upgraded gson version with unit test #4057

[GOBBLIN-2158] upgraded gson version with unit test #4057

Blazer-007 commented Sep 17, 2024

pratapaditya04 Sep 18, 2024 •

edited

Loading

Will-Lo Sep 18, 2024

pratapaditya04 Sep 18, 2024

pratapaditya04 Sep 18, 2024

pratapaditya04 Sep 18, 2024 •

edited

Loading

pratapaditya04 Sep 18, 2024 •

edited

Loading

Blazer-007 Sep 18, 2024

Will-Lo Sep 18, 2024

Blazer-007 commented Sep 18, 2024

pratapaditya04 commented Sep 18, 2024

Will-Lo commented Sep 18, 2024

Blazer-007 commented Sep 19, 2024

[GOBBLIN-2158] upgraded gson version with unit test #4057

[GOBBLIN-2158] upgraded gson version with unit test #4057

Conversation

Blazer-007 commented Sep 17, 2024

JIRA

Description

Tests

Commits

pratapaditya04 Sep 18, 2024 • edited Loading

Choose a reason for hiding this comment

Will-Lo Sep 18, 2024

Choose a reason for hiding this comment

pratapaditya04 Sep 18, 2024

Choose a reason for hiding this comment

pratapaditya04 Sep 18, 2024

Choose a reason for hiding this comment

pratapaditya04 Sep 18, 2024 • edited Loading

Choose a reason for hiding this comment

pratapaditya04 Sep 18, 2024 • edited Loading

Choose a reason for hiding this comment

Blazer-007 Sep 18, 2024

Choose a reason for hiding this comment

Will-Lo Sep 18, 2024

Choose a reason for hiding this comment

Blazer-007 commented Sep 18, 2024

pratapaditya04 commented Sep 18, 2024

Will-Lo commented Sep 18, 2024

Blazer-007 commented Sep 19, 2024

pratapaditya04 Sep 18, 2024 •

edited

Loading

pratapaditya04 Sep 18, 2024 •

edited

Loading

pratapaditya04 Sep 18, 2024 •

edited

Loading